Fix multi-release JAR detection on Windows and add integration test - #563
Conversation
9f77b32 to
98441ac
Compare
elharo
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a Maven Invoker integration test to validate that the built JAR is marked as multi-release (Multi-Release: true) when META-INF/versions/ content is present, improving cross-platform confidence (including Windows).
Changes:
- Added a new Maven Invoker IT project (
MJAR-557-detect-mjar) that packagesMETA-INF/versions/9/...content. - Added a
verify.groovyscript to assertMulti-Releasemanifest attribute is set totrue. - Updated root
pom.xmlexcludes to avoid RAT/header checks on the added plain text fixture.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/it/MJAR-557-detect-mjar/verify.groovy | Verifies the built JAR manifest contains Multi-Release: true. |
| src/it/MJAR-557-detect-mjar/src/main/resources/META-INF/versions/9/test.txt | Adds a META-INF/versions/9/ fixture to trigger MRJAR detection. |
| src/it/MJAR-557-detect-mjar/pom.xml | Defines the IT Maven project that builds a JAR using the plugin under test. |
| src/it/MJAR-557-detect-mjar/invoker.properties | Configures Invoker execution constraints and test description. |
| pom.xml | Excludes the fixture text file from checks (e.g., RAT/license header validation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed Copilot review comments:
|
slachiewicz
left a comment
slachiewicz
left a comment
There was a problem hiding this comment.
General looks good.
There are unresolved copilot issues.
gnodet
changed the title
|
Added the actual code fix in the latest commit: replaced The IT (with The |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The bug fix is correct, but there is a critical integration problem that will need to be resolved before this can merge.
Conflict with bcae92d (#508)
The createArchive() method that this PR patches no longer exists on master. Commit bcae92d (Migrate from Maven Archiver to standard jar tool, PR #508) replaced the entire FileSetManager-based code path with a FileCollector approach:
// master — createArchives() uses FileCollector, no FileSetManager at all
var files = new FileCollector(this, executor, classesDirectory, matcherFactory);
Files.walkFileTree(classesDirectory, files);FileCollector uses Path.endsWith(VERSIONS) to detect META-INF/versions/ directories (line 289 of FileCollector.java):
if (detectMultiReleaseJar && directory.endsWith(VERSIONS)) {Path.endsWith(String) compares path components, not raw strings, so it is already OS-independent — the Windows File.separatorChar bug this PR fixes does not exist on master. GitHub confirms this: mergeableState: dirty.
Action required: rebase this branch on master. After the rebase, the AbstractJarMojo.java change will conflict and should be dropped — the fix is already in place via FileCollector. The IT (MJAR-557-detect-mjar) is still valuable and should be kept as-is; it exercises the real code path and adds cross-platform confidence.
Prior review comments
The Copilot findings (JarFile not closed, missing assertion message) have both been addressed — withCloseable is in place and the assertion includes a diagnostic message. slachiewicz's CHANGES_REQUESTED can be re-evaluated after the rebase once the conflict is resolved.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after the latest two commits (d6b5c31, 3634093).
Prior findings — all resolved
Previous bot review (COMMENT on d6b5c31503ed): flagged a merge conflict: the createArchive() / FileSetManager code path no longer exists on master after PR #508 replaced it with a FileCollector / jar-tool architecture.
Resolved by 3634093: the merge conflict was properly resolved by taking master's AbstractJarMojo (with the new FileCollector architecture) and dropping the File.separatorChar fix — which is moot since FileCollector.preVisitDirectory() uses Path.endsWith(VERSIONS) (path-component comparison, already OS-agnostic).
slachiewicz's CHANGES_REQUESTED: the two flagged items (JarFile not closed; assertion without message) were addressed in a52ae29. The test.txt fixture comment was answered by elharo — any entry under META-INF/versions/ triggers detection, not just .class files. The concerns are resolved.
Current diff review
The current diff is integration-test only. No production code is modified.
verify.groovy:withCloseablehandles theJarFileresource leak; assertion includes a diagnostic message. Correct.invoker.java.version = 9+: appropriate guard for multi-release JAR support.- IT fixture:
HelloWorld.javaprovides a base class, ensuringMETA-INF/versions/9/stays as a versioned layer under--release 9and is not promoted to base byArchive.prune(). The jar tool then setsMulti-Release: trueautomatically. Logic is sound. pom.xmlcheckstyle exclusion: path matches exactly; needed becausetest.txthas no license header and should not be checked.
No issues found. CI is pending (not failing).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
- Fix multi-release JAR detection on Windows: FileSetManager.getIncludedFiles() returns forward-slash paths regardless of OS; replace File.separatorChar with literal '/' to match the JAR spec and FileSetManager normalization - Add MJAR-557 integration test that verifies Multi-Release: true is set in the manifest when META-INF/versions/ files are present - Update IT for new FileCollector architecture: FileCollector uses Path comparisons, so the Windows bug is already fixed on master; add HelloWorld.java base class so the versioned entry is correctly handled with --release 9 Fixes: MJAR-557
Adds an integration test that verifies the
Multi-Release: truemanifest entry is added when the class output containsMETA-INF/versions/files.The detection logic in
AbstractJarMojousesFile.separatorCharto build the path prefix, which matches the platform separator used byDirectoryScanner. This test passes on all platforms (Linux, macOS, Windows).